Moving Microsoft.ServiceModel.Syndication from wcf to corefx. #24202

shmao · 2017-09-21T21:52:38Z

The PR is moving Microsoft.ServiceModel.Syndication from wcf to corefx.

…eading a feed from outside, also as part of our default date parser, if it can't parse the date it will just assign a default date to avoid the crash

Added custom parsing for RSS formats. Cleaned the code. Executed CodeFormatter tool. Added test for custom parsing.

…or RSS formatting Image issue fixed and added some tests

- Change M.SM.SyndicationFeed to be a .NET Standard 2.0 library - Change tests to use official .NET Core 2.0 release from preview

danmoseley · 2017-09-21T21:59:32Z

What is the reason for moving it to corefx? The compat pack can pull it just as well from wcf.

Right now we are trying to figure out some rules about what libraries we want to add to the corefx repo. I am not sure whether this is one or not.

@Petermarcu @karelz

[EDIT] Fixing Peter's alias

justinvp · 2017-09-21T22:23:30Z

I'm curious: Why is this Microsoft.ServiceModel.Syndication and not System.ServiceModel.Syndication to match .NET Framework?

zhenlan · 2017-09-21T23:24:34Z

What is the reason for moving it to corefx? The compat pack can pull it just as well from wcf.

Hi @danmosemsft The Syndication library has no dependency on WCF at all. It does not really related to WCF either. Many other these kind of libraries live in corefx repo today. Corefx repo has better build support etc. so we can be more focusing on the code and get the release out of door more efficiently. In the future, if we have a better plan for all the projects that live in corefx but does not belong to the shared framework, we certainly can follow any guidance at that time.

Why is this Microsoft.ServiceModel.Syndication and not System.ServiceModel.Syndication to match .NET Framework?

Hi @justinvp Yes, it was given "Microsoft" namespace while we were prototyping the port. It will be changed to "System" namespace before the release.

justinvp · 2017-09-21T23:32:33Z

Yes, it was given "Microsoft" namespace while we were prototyping the port. It will be changed to "System" namespace before the release.

Should it be changed to System before it lands in corefx (i.e. change to System as part of this PR)?

shmao · 2017-09-21T23:34:51Z

Should it be changed to System before it lands in corefx (i.e. change to System as part of this PR)?

Sounds good. Will do.

karelz · 2017-09-22T00:17:18Z

I share @danmosemsft's concern about adding this library into corefx instead of wcf repo.
What is the motivation? Isn't the entire System.ServiceModel namespace heavily WCF related?

~~BTW: What is the value of using Microsoft namespace for the prototype? (I can't come up with any)~~
UPDATE: It is already renamed, so irrelevant question.

justinvp · 2017-09-22T00:30:47Z

src/System.ServiceModel.Syndication/Microsoft.ServiceModel.Syndication.sln

+MinimumVisualStudioVersion = 10.0.40219.1
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ServiceModel.Syndication", "src\Microsoft.ServiceModel.Syndication.csproj", "{B615835F-C286-4FB5-A2BF-C3B967AB9EC6}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.ServiceModel.Syndication.Tests", "tests\Microsoft.ServiceModel.Syndication.Tests.csproj", "{A622B2C0-DD74-4218-9CF0-F9B2E52F4E91}"


I'm sure you're aware but the project references and name of this solution file should be changed from Microsoft to System.

The csproj currently does not build in corefx. The PR is mainly for porting the code from wcf to corefx. I planned to enable the build of the project later and fix the namespace later for better code review experience.

justinvp · 2017-09-22T00:30:51Z

src/System.ServiceModel.Syndication/src/App10Constants.cs

+// See the LICENSE file in the project root for more information.
+
+
+namespace Microsoft.ServiceModel.Syndication


Microsoft -> System (applies throughout)

justinvp · 2017-09-22T00:30:54Z

src/System.ServiceModel.Syndication/src/App10Constants.cs

+
+namespace Microsoft.ServiceModel.Syndication
+{
+    internal static class App10Constants


To be consistent with the other components in corefx, the source files should be nested under their namespace components (for example, see System.Collections' layout).

Instead of:

src/System.ServiceModel.Syndication/src/App10Constants.cs

Change to:

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/App10Constants.cs

(applies throughout)

Yes, will change this later in separate PR.

shmao · 2017-09-22T00:34:59Z

Isn't the entire System.ServiceModel namespace heavily WCF related?

Although S.SM.Syndication is in System.ServiceModel namespace, it's not related to WCF. It actually has no dependency on any of WCF components at all. I believed it was put in System.ServiceModel only because it was developed by WCF team.

stephentoub · 2017-09-22T01:20:24Z

I believed it was put in System.ServiceModel only because it was developed by WCF team.

Is there anything else System.ServiceModel.* in corefx? Seems pretty explainable to say "If the thing you're looking for is in System.ServiceModel, it's in the wcf repo".

zhenlan

There are will be additional PRs to make this project build in corefx, clean up namespace, add more tests etc. etc. Once this PR is merged it will serve as a baseline. It will make it easier to review future PRs. We can create an issue to track any specific concerns folks may have now and address them in future PRs.

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. Commit migrated from dotnet/corefx@feb04e0

mconnew and others added 30 commits June 7, 2017 18:03

Microsoft.ServiceModel.Syndication skeleton project

e48777c

Adding the existing classes of SyndicationFeed from .net fx

1d9f27f

Added the needed references to get the code to compile

9c278a1

Changed some namespaces

aa05946

Fixed errors when reading feeds and replaced some buffers

398649d

Cleaning code for PR

0594aff

Deleted some unused files

9f6c78e

Added the posibility that the user creates his own date parser when r…

37bf505

…eading a feed from outside, also as part of our default date parser, if it can't parse the date it will just assign a default date to avoid the crash

Added correct testnames and Copyright

62f2980

Initial changes to add custom parsers

91a06a4

Added more delegates as parsers

adcb15b

Added custom parser delegates

01e52b9

Save changes

7ece449

Initial SyndicationFeed

898bff5

Removed the dependence of the old SR class

6f2b69b

Cleaned the code, deleted Diagnostics, fixed throwing resources

c7be030

Cleaned code from most of the unnecesary comments

709a1ab

Formatted the code with CodeFormatter Tool

8d9968f

Moved the call of itemParser to be called with each item

ab8c7ae

Test using custom parsers

681f0fb

Merge branch 'CustomParser' into InitialSyndicationFeed

ae10738

Added custom parsing for RSS formats. Cleaned the code. Executed CodeFormatter tool. Added test for custom parsing.

Fixed image issue where image tag was using original title and url, f…

25309a2

…or RSS formatting Image issue fixed and added some tests

Test clase jorge

b4d2d1b

Save changes with Jorge cass

21f972f

Initial Jorge

5dacea0

saving changes

77738fd

Save changes

28b6ef6

Fixed image and items issue

52a29fd

Fixed disjoint items in atom

0f514e5

Run codeFormatter

a8a8dfe

ElJerry and others added 5 commits July 31, 2017 10:48

Fixed sync method exposed GetReader.

8df46f7

Edited XmlReaderWrapper, moved methods to extension methods.

2f16e88

Removed unnecesary variables from Wrappers.

755906b

Fixed bug from ServiceModel.Syndication

1752cc9

Fix ServiceModel.SyndicationFeed project dependencies

594afa6

- Change M.SM.SyndicationFeed to be a .NET Standard 2.0 library - Change tests to use official .NET Core 2.0 release from preview

dnfclas added the cla-already-signed label Sep 21, 2017

shmao requested review from zhenlan and mconnew September 21, 2017 21:53

shmao added 3 commits September 21, 2017 16:36

Move Syndication to root/src

c7b485f

Merge branch 'lab' of C:\GitMV\wcf into AddSyndication

0115259

Disable Build of S.SM.Syndication.

e35e0cb

shmao force-pushed the AddSyndication branch from 23e0a1b to e35e0cb Compare September 21, 2017 23:44

karelz added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-Meta labels Sep 22, 2017

justinvp reviewed Sep 22, 2017

View reviewed changes

karelz removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 22, 2017

zhenlan approved these changes Sep 22, 2017

View reviewed changes

shmao merged commit feb04e0 into dotnet:master Sep 22, 2017

karelz added this to the 2.1.0 milestone Oct 11, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#24202 from shmao/AddSyndication

442e2ba

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. Commit migrated from dotnet/corefx@feb04e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. #24202

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. #24202

shmao commented Sep 21, 2017

danmoseley commented Sep 21, 2017 •

edited by karelz

Loading

justinvp commented Sep 21, 2017

zhenlan commented Sep 21, 2017

justinvp commented Sep 21, 2017

shmao commented Sep 21, 2017

karelz commented Sep 22, 2017 •

edited

Loading

justinvp Sep 22, 2017

shmao Sep 22, 2017

justinvp Sep 22, 2017

justinvp Sep 22, 2017

shmao Sep 22, 2017

shmao commented Sep 22, 2017

stephentoub commented Sep 22, 2017

zhenlan left a comment

		// See the LICENSE file in the project root for more information.


		namespace Microsoft.ServiceModel.Syndication

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. #24202

Moving Microsoft.ServiceModel.Syndication from wcf to corefx. #24202

Conversation

shmao commented Sep 21, 2017

danmoseley commented Sep 21, 2017 • edited by karelz Loading

justinvp commented Sep 21, 2017

zhenlan commented Sep 21, 2017

justinvp commented Sep 21, 2017

shmao commented Sep 21, 2017

karelz commented Sep 22, 2017 • edited Loading

justinvp Sep 22, 2017

Choose a reason for hiding this comment

shmao Sep 22, 2017

Choose a reason for hiding this comment

justinvp Sep 22, 2017

Choose a reason for hiding this comment

justinvp Sep 22, 2017

Choose a reason for hiding this comment

shmao Sep 22, 2017

Choose a reason for hiding this comment

shmao commented Sep 22, 2017

stephentoub commented Sep 22, 2017

zhenlan left a comment

Choose a reason for hiding this comment

danmoseley commented Sep 21, 2017 •

edited by karelz

Loading

karelz commented Sep 22, 2017 •

edited

Loading